PG-1923 WIP 256-bit encryption (take 2) #587
Conversation
It's a draft. Only Principal and WAL keys work so far
dAdAbird
force-pushed
the
32_byte_keys_take2
branch
from
c012cde to
0b8794c
Compare
| AesEcbEncrypt(EVP_CIPHER_CTX **ctxPtr, const unsigned char *key, int key_len, const unsigned char *in, int in_len, unsigned char *out) | ||
| { | ||
| int out_len; | ||
| const EVP_CIPHER *cipher = key_len == 32 ? cipher_ctr_ecb_256 : cipher_ctr_ecb; |
There was a problem hiding this comment.
As for me it needs better validation here, or may be even accept some enum value here instead of key_len as we always expect discrete values.
| NULL /* show_hook */ | ||
| ); | ||
|
|
||
| DefineCustomEnumVariable("pg_tde.cipher", /* name */ |
There was a problem hiding this comment.
I'm pretty sure that over time we will have request for multi-tenant cipher configuration. Not sure, but maybe it worth to think how to simplify our future. Maybe call it pg_tde.default_cipher?
There was a problem hiding this comment.
Maybe, I'm not sure. It might be confusing right now - if that's a default then what else could be set. Besides the real default is "aes128" - which is if the GUC isn't defined :)
But a good point, need to think about that
| } | ||
|
|
||
| void | ||
| pg_tde_update_wal_keys_file(void) |
There was a problem hiding this comment.
I think something like migrate fits better to this function name rather than update
|
|
||
| typedef struct WalKeyFileEntry | ||
| { | ||
| uint32 key_len; |
There was a problem hiding this comment.
I wonder if it's worth to store cipher type here as well for future needs?
| } | ||
| pg_tde_save_server_key(principalKey, false); | ||
| TDEXLogSmgrInitWrite(true); | ||
| TDEXLogSmgrInitWrite(true, 16); |
There was a problem hiding this comment.
It's pinned here just for time being?
There was a problem hiding this comment.
yes, I haven't look at tools yet, so it's a stub just to compile. The basebackup and other (?) tools that creates key, probably would have to have it's own flag for the key size/cipher
It does make sense to rewrite old files on server start (otherwise it'll slow down random read/write). But reading all _keys files and checking the magic number could slow the server start, especially if there are a lot of databases with encrypted tables. Moreover, we'd rewrite files once but would have to scan and read them on every start... That's why this commit introduces new suffixes to the filenames in the new format. That way, we would only scan the dir and read file names, instead of opening and reading each _keys file.
Including migration
dAdAbird
force-pushed
the
32_byte_keys_take2
branch
from
b851c0f to
1ebc59a
Compare
|
|
||
| typedef struct TDEMapEntry | ||
| { | ||
| uint32 key_len; /* Part of AAD */ |
There was a problem hiding this comment.
Don't we want to encode which cipher was used? And do we want to support ciphers with variable IV lengths?
| #define PG_TDE_MAP_OLD_FNAME_SUFFIX "_keys" | ||
|
|
||
| #define PG_TDE_FILEMAGIC 0x04454454 /* version ID value = TDE 04 */ | ||
| #define PG_TDE_MAP_FILENAME "%d_keys_v2" /* TODO: should be _v2 of _v4 ? */ |
There was a problem hiding this comment.
Doesn't having a different suffix complicate life for tools which work with the files, e.g. pg_basebackup?
| /* | ||
| * TODO: An ugly hack for now, we need to get a key info when rewriting | ||
| * an old file... | ||
| */ |
There was a problem hiding this comment.
Wouldn't it make more sense to have this as a separate function which is called by the migration code?
3 participants
SMGR keys don't work yet.This is a draft and needs further improvements and refactoring. I just want feedback on the general direction.
Cheers